Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Suppress "const" prefix of FnDef constants in MIR dump #75697

Merged
merged 1 commit into from
Aug 21, 2020
Merged

Suppress "const" prefix of FnDef constants in MIR dump #75697

merged 1 commit into from
Aug 21, 2020

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Aug 19, 2020 •
edited
Loading

I was asked to suppress the const infront of FnDef.
I tried to suppress comments for other types, but turned out that const () and () is different: #75697 (comment)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 19, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Aug 19, 2020

cc @mark-i-m Do I need to update MIR dump in rustc-dev-guide somewhere?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 19, 2020

cc @rust-lang/wg-mir-opt

tesuji
tesuji commented Aug 19, 2020
View reviewed changes
src/test/ui/consts/const-extern-fn/const-extern-fn-call-extern-fn.stderr
@@ -1,4 +1,4 @@
error[E0723]: can only call other `const fn` within a `const fn`, but `const regular_in_block` is not stable as `const fn`
error[E0723]: can only call other `const fn` within a `const fn`, but `regular_in_block` is not stable as `const fn`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this is interesting!

@bors
Copy link
Contributor

bors commented Aug 19, 2020

☔ The latest upstream changes (presumably #75715) made this pull request unmergeable. Please resolve the merge conflicts.

@tesuji tesuji force-pushed the mir-dumb-const-prefix branch 3 times, most recently from 1941082 to 4089aad Compare August 20, 2020 11:04
@tesuji
Copy link
Contributor Author

tesuji commented Aug 20, 2020

cc @bjorn3 for your work with MIR in cg_cranelift .

bjorn3
bjorn3 reviewed Aug 20, 2020
View reviewed changes
src/test/mir-opt/address_of.borrow_and_cast.SimplifyCfg-initial.after.mir Outdated
@@ -38,7 +38,7 @@ fn borrow_and_cast(_1: i32) -> () {
_6 = &raw mut (*_7); // scope 2 at $DIR/address-of.rs:44:13: 44:19
FakeRead(ForLet, _6); // scope 2 at $DIR/address-of.rs:44:9: 44:10
StorageDead(_7); // scope 2 at $DIR/address-of.rs:44:31: 44:32
_0 = const (); // scope 0 at $DIR/address-of.rs:41:32: 45:2
_0 = (); // scope 0 at $DIR/address-of.rs:41:32: 45:2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a difference between these two. const () is a Constant, while () is an Aggregate.

src/test/mir-opt/array_index_is_temporary.main.SimplifyCfg-elaborate-drops.after.mir.32bit Outdated
@@ -25,9 +25,9 @@ fn main() -> () {

bb0: {
StorageLive(_1); // scope 0 at $DIR/array-index-is-temporary.rs:13:9: 13:14
_1 = [const 42_u32, const 43_u32, const 44_u32]; // scope 0 at $DIR/array-index-is-temporary.rs:13:17: 13:29
_1 = [42_u32, 43_u32, 44_u32]; // scope 0 at $DIR/array-index-is-temporary.rs:13:17: 13:29
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarily here, though I am not sure if the Constant could be printed as an array, or would be printed as a raw allocation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it can be printed as an array, so yea, there would be no visible difference in the mir

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether the information const vs Aggregate is useful, but the original comment by me that motivated @lzutao to open this PR was solely for FnDef, where the const is actively confusing (at least in the position for function calls. Maybe we should just special case function calls to not print a const?

Copy link
Member

@wesleywiser wesleywiser Aug 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel super strongly but I would prefer if this was only done for FnDefs right now.

@tesuji tesuji changed the title Suppress "const" prefix of constants in MIR dump Suppress "const" prefix of FnDef constants in MIR dump Aug 20, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Aug 20, 2020

Thanks for all comments. Now I only suppress "const" prefix for FnDef.

@tesuji tesuji marked this pull request as ready for review August 20, 2020 12:36
@bors
Copy link
Contributor

bors commented Aug 20, 2020

☔ The latest upstream changes (presumably #75670) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 20, 2020

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Aug 20, 2020

📌 Commit 4019330ee337e28ac2869ebbc47884f819c269b2 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Aug 20, 2020

Sorry, forgot to edit the commit message. It should be more correct now.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 20, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Aug 20, 2020

📌 Commit 3b853f396dcbcbb5ddf94373526e7598184cee1f has been approved by oli-obk

@bors
Copy link
Contributor

bors commented Aug 20, 2020

☔ The latest upstream changes (presumably #75562) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 20, 2020
@mark-i-m
Copy link
Member

cc @mark-i-m Do I need to update MIR dump in rustc-dev-guide somewhere?

To be honest, I'm not sure. I don't remember how much we have about MIR dump. Perhaps someone else from @rust-lang/wg-rustc-dev-guide knows?

@JohnTitor
Copy link
Member

@lzutao @mark-i-m We have https://rustc-dev-guide.rust-lang.org/mir/debugging.html but it doesn't seem to be affected.

@wesleywiser
Copy link
Member

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Aug 21, 2020

📌 Commit c4c017a has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2020
@bors
Copy link
Contributor

bors commented Aug 21, 2020

⌛ Testing commit c4c017a with merge fbc600c60e44097b00ddc90cee2548c4e59d7610...

@bors
Copy link
Contributor

bors commented Aug 21, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 21, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Aug 21, 2020

Spurious error,

warning: spurious network error (2 tries remaining): [28] Timeout was reached (failed to download any data for `quick-error v1.2.3` within 30s)
warning: spurious network error (1 tries remaining): [28] Timeout was reached (failed to download any data for `quick-error v1.2.3` within 30s)
error: failed to download from `https://crates.io/api/v1/crates/quick-error/1.2.3/download`

Caused by:
  [28] Timeout was reached (failed to download any data for `quick-error v1.2.3` within 30s)

@oli-obk
Copy link
Contributor

oli-obk commented Aug 21, 2020

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2020
@bors
Copy link
Contributor

bors commented Aug 21, 2020

⌛ Testing commit c4c017a with merge b374559b5a3ce28efb78626eab441eb0c1e176d6...

@bors
Copy link
Contributor

bors commented Aug 21, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 21, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Aug 21, 2020

this appears to break rustfmt?

@JohnTitor
Copy link
Member

Hm, I don't think it's caused by this PR, though I'm not sure if it's spurious or not.

2020-08-21T08:54:27.4115722Z �[0m�[1m�[38;5;10mnote�[0m�[0m�[1m�[38;5;15m: `link.exe` returned an unexpected error�[0m
2020-08-21T08:54:27.4116018Z 
2020-08-21T08:54:27.4116240Z �[0m�[1m�[38;5;10mnote�[0m�[0m�[1m�[38;5;15m: the Visual Studio build tools may need to be repaired using the Visual Studio installer�[0m
2020-08-21T08:54:27.4116388Z 
2020-08-21T08:54:27.4116584Z �[0m�[1m�[38;5;10mnote�[0m�[0m�[1m�[38;5;15m: or a necessary component may be missing from the "C++ build tools" workload�[0m
2020-08-21T08:54:27.4116722Z 
2020-08-21T08:54:27.4128039Z �[0m�[1m�[38;5;9merror�[0m�[0m�[1m�[38;5;15m: aborting due to previous error�[0m
2020-08-21T08:54:27.4128217Z 
2020-08-21T08:54:27.4220384Z [RUSTC-TIMING] build_script_build test:false 2.622
2020-08-21T08:54:27.4233457Z �[0m�[0m�[1m�[31merror�[0m�[1m:�[0m could not compile `stacker`.
2020-08-21T08:54:27.4233664Z 
2020-08-21T08:54:27.4233931Z To learn more, run the command again with --verbose.
2020-08-21T08:54:27.4234190Z �[0m�[0m�[1m�[33mwarning�[0m�[1m:�[0m build failed, waiting for other jobs to finish...
2020-08-21T08:54:28.2375366Z [RUSTC-TIMING] build_script_build test:false 3.434
2020-08-21T08:54:31.5274931Z �[0m�[0m�[1m�[31merror�[0m�[1m:�[0m build failed
2020-08-21T08:54:31.5345517Z command did not execute successfully: "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage0\\bin\\cargo.exe" "build" "--target" "x86_64-pc-windows-msvc" "-Zbinary-dep-depinfo" "-j" "8" "--release" "--locked" "--color" "always" "--manifest-path" "D:\\a\\rust\\rust\\src/tools/rustfmt\\Cargo.toml" "--features" "rustc-workspace-hack/all-static" "--message-format" "json-render-diagnostics"
2020-08-21T08:54:31.5346495Z expected success, got: exit code: 101
2020-08-21T08:54:31.5368358Z [TIMING] ToolBuild { compiler: Compiler { stage: 2, host: TargetSelection { triple: "x86_64-pc-windows-msvc", file: None } }, target: TargetSelection { triple: "x86_64-pc-windows-msvc", file: None }, tool: "rustfmt", path: "src/tools/rustfmt", mode: ToolRustc, is_optional_tool: true, source_type: Submodule, extra_features: [] } -- 15.722
2020-08-21T08:54:31.5368933Z failed to test rustfmt: could not build

@mati865
Copy link
Contributor

mati865 commented Aug 21, 2020

Has to be spurious: error: linking with link.exe failed: exit code: 0xc0000005
Changes in this PR are extremely unlikely to crash MSVC linker.

@JohnTitor
Copy link
Member

Thanks, @mati865. And the builder on #75765 passes so let's try it again.
@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2020
@bors
Copy link
Contributor

bors commented Aug 21, 2020

⌛ Testing commit c4c017a with merge 521db88...

@bors
Copy link
Contributor

bors commented Aug 21, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 521db88 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 21, 2020
@bors bors merged commit 521db88 into rust-lang:master Aug 21, 2020
@tesuji tesuji deleted the mir-dumb-const-prefix branch August 21, 2020 13:33
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@wesleywiser wesleywiser wesleywiser left review comments

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@oli-obk oli-obk

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.47.0
Development

Successfully merging this pull request may close these issues.